Added `findaxis`, `finddim`, and `hasdim` #89

Tokazama · 2019-09-06T08:43:14Z

These are zero allocation methods that should allow easy development of methods based on the axis names. For example, defining timedim with this implementation would only require timedim(x) = finddim(x, :time).

These are zero allocation methods that should allow easy development of axis name based methods.

codecov · 2019-09-06T09:07:38Z

Codecov Report

Merging #89 into master will increase coverage by 0.94%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage    67.6%   68.55%   +0.94%     
==========================================
  Files           9        9              
  Lines         426      442      +16     
==========================================
+ Hits          288      303      +15     
- Misses        138      139       +1

Impacted Files	Coverage Δ
src/ImageCore.jl	`61.11% <ø> (ø)`	⬆️
src/traits.jl	`86.59% <93.75%> (+1.41%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e8a382...6d273bd. Read the comment docs.

codecov · 2019-09-06T09:07:38Z

Codecov Report

Merging #89 into master will increase coverage by 1.65%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage    67.6%   69.26%   +1.65%     
==========================================
  Files           9        9              
  Lines         426      449      +23     
==========================================
+ Hits          288      311      +23     
  Misses        138      138

Impacted Files	Coverage Δ
src/ImageCore.jl	`61.11% <ø> (ø)`	⬆️
src/traits.jl	`88.46% <100%> (+3.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e8a382...06ea089. Read the comment docs.

timholy

I'm generally approving but you'll see there are some design issues to think about.

timholy · 2019-09-06T10:53:26Z

src/traits.jl

+    if hasdim_noerror(syms, name)
+        return getfield(nt, name)
+    else
+        return 0:0


0:0 is not empty (it has one item). I think you mean 0:-1.

That's is what I was going for but if there's something more rational to fallback on I'm all ears.

timholy · 2019-09-06T10:56:35Z

src/traits.jl

+Returns the axis (as in `axes(img, dim)`) that corresponds to the `name`. If no
+matching name is found any empy axis (i.e. `0:0`) is returned.
+"""
+@inline findaxis(A::T, name::Symbol) where {T} = _findaxis(namedaxes(A), name)


::T where T is extraneous here.

timholy · 2019-09-06T11:00:11Z

src/traits.jl

+_hasdim(::HasDimNames{true}, x::T, name::Symbol) where {T} = hasdim_noerror(names(x), name)
+_hasdim(::HasDimNames{false}, x::T, name::Symbol) where {T} = false
+
+Base.@pure function hasdim_noerror(dimnames::Tuple{Vararg{Symbol, N}}, name::Symbol) where N


The name here is slightly odd. You just want it to be different from hasdim I guess?

I adapted the method from what I found in NamedDims and kind of stuck with the naming. My goal was to isolate it so it didn't have any other generic methods. I'm never sure if I'm going to mess up use of @pure because I've read very different explanations of how it should be used from experienced users. I'd be happy to make it consistent with the other methods if it's not a problem.

timholy · 2019-09-06T11:03:59Z

src/traits.jl

+"""
+hasdim(x::T, name::Symbol) where {T} = _hasdim(HasDimNames(T), x, name)
+_hasdim(::HasDimNames{true}, x::T, name::Symbol) where {T} = hasdim_noerror(names(x), name)
+_hasdim(::HasDimNames{false}, x::T, name::Symbol) where {T} = false


finddim(A, :dim_1) could work even if !hasdim(A, :dim_1). I worry a bit about API confusion/consistency in such cases. How important is it to have default names? Should we eliminate them and support operations only on explicitly-named axes?

Good point. That needs to be more consistent. It seems kind of pointless to have it act on a default name that has no significant meaning. This makes me think that if there are default names they should probably be more meaningful, like those in AxisArrays. It may also help users if there's consistency with how AxisArrays names worked. I don't have a strong opinion either way, as long as I don't end being the guy that breaks JuliaImages.

I'm not sure I'm a fan of :row, :col, and :page either. Those don't sound like image properties. At best they are linear algebra properties; just because we use 2d arrays for both doesn't mean image == matrix. (How often do you multiply an image times a vector??)

Can we just drop the default names altogether, and have these only operate on arrays with explicitly-assigned names?

vote for :dim_1, :dim_2, and :dim_3

* New (old AxisArray) default names * More logical internal names (e.g., `_hasdim`)

1. The internal names now follow the general rule that the first internal layer of a method is "_methodname" and the second is "__methodname". This makes the internal vs public api more specific. 2. Added tests for each default name. Hopefully this gives 100% coverage for the trait interfaces methods.

timholy

If it's possible to just get rid of the default names, I think we can merge this. If it's necessary to keep them, please elaborate on an actual use-case for which this is a make-or-break deal.

timholy · 2019-09-16T14:10:07Z

src/traits.jl

-function namedaxes(::HasDimNames{false}, img::AbstractArray{T,N}) where {T,N}
-    NamedTuple{default_names(Val(N))}(axes(img))
+Returns the axis (as in `axes(img, dim)`) that corresponds to the `name`. If no
+matching name is found any empy axis (i.e. `0:0`) is returned.


Suggested change

matching name is found any empy axis (i.e. `0:0`) is returned.

matching name is found any empty axis (i.e. `0:-1`) is returned.

I almost wonder if axisrange is a better name for this operation?

In this case would you be thinking of the axis as the names and range as the tuple of axes?

I'm just trying to ask myself, "what would I expect findaxis to do?" And I am not sure. findmin returns both the value and location of the minimum. So maybe findaxis should return the value (i.e., range) and dimension index (like finddim)? Or maybe axisvalue, which is again back to the names chosen by AxisArrays.

I guess I'm trying to imitate the behavior of axes(img, i) where i would be a symbol. The trick is that whatever type img is may not wrap axes that are also directly linked to their names (e.g., one array type stores names wraps another type that stores axes). So if we can access the axes and names independently then we can use namedaxes to connect these otherwise independent features.

I think the rationale of findaxis returning something similar to findmin makes a lot of sense though. In this case maybe findaxis would return something like NamedTuple{(:dim_1)}((1:10,)) and axisvalue would return the individual axis/range.

I could go with that latter proposal.

timholy · 2019-09-16T14:19:45Z

I know that default_names has already made it in, but since there are no users in JuliaImages I think it's fine to take it out again if we need to.

One suggestion: back when JuliaImages was mostly a one-person show, I was writing the algorithms at the same time as deciding about traits, and the efforts of implementing the algorithms forced me to think clearly about what I actually needed. (Not that I always got the design right, mind you!) You may be doing the same thing, so when you encounter an example, don't hesitate to post it in a comment or gist. It would help make sure we are thinking about the same thing.

Tokazama · 2019-09-16T15:31:12Z

The downside to not using defaults_names is that any method that uses these
traits would have to account for a different type. With the current
implementation most of ImageAxes traits could be written as:

colordim(img) = finddim(img, :color)
coloraxis(img) = findaxis(img, :color)

timedim(img) = finddim(img, :time)
timeaxis(img) = findaxis(img, :time)

Without defaults it would either throw an error or the default behavior would
have to be described at the level of the method.

@inline finddim(A::T, name::Symbol) where {T} = _finddim(HasDimNames(T), A, name)
_finddim(::HasDimNames{false}, A::T, name::Symbol) where {T} = nothing
_finddim(::HasDimNames{true}, A::T, name::Symbol) where {T} = __finddim(names(A), name)

function timedim(img)
    d = finddim(img)
    if isnothing(d)
        return 0
    else
        return d
    end
end

I don't think there's anything wrong with this approach, it's just more verbose.

timholy · 2019-09-16T16:00:02Z

I kind of like the verbosity. And why couldn't timedim(img) = finddim(img, :time)? Why is 0 better than nothing?

Tokazama · 2019-09-16T16:34:49Z

The main reason was so that it always returned something of the same type, but that's only an issue if img is inferred to be a union of types that can return HasDimNames{true} or HasDimNames{false}. So the only situation where it would be useful is something like:

d = (NamedDimsArray, Array)

timedim(d[1])

Tokazama · 2019-09-16T16:37:08Z

Actually, returning nothing might be a nice way of allowing methods like timedim to more easily implement custom defaults or errors when HasDimNames{false} without throwing an error. That's hard to do if everything is Int no matter what

Tokazama · 2019-12-17T02:26:10Z

It looks like the main methods this PR was for (findaxis, finddim, and hasdim) will be handled by whatever ends up being the backend for our arrays (maybe NamedDims). Would it make sense to close this PR and move efforts there.

Tokazama · 2020-04-23T13:11:51Z

Unless there are any objections, I'm going to close this and probably open a more disruptive but complete PR using more recent developments for axes and named dimensions.

Added findaxis, finddim, and hasdim

6d273bd

These are zero allocation methods that should allow easy development of axis name based methods.

timholy reviewed Sep 6, 2019

View reviewed changes

Tokazama added 2 commits September 11, 2019 10:21

Fix inconsistencies in trait behavior

ce50f71

* New (old AxisArray) default names * More logical internal names (e.g., `_hasdim`)

timholy reviewed Sep 16, 2019

View reviewed changes

Tokazama mentioned this pull request Sep 17, 2019

WIP: The Plan JuliaCollections/AxisArraysFuture#1

Open

timholy mentioned this pull request Nov 23, 2019

Used Base.names invenia/NamedDims.jl#22

Closed

Tokazama closed this Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `findaxis`, `finddim`, and `hasdim` #89

Added `findaxis`, `finddim`, and `hasdim` #89

Tokazama commented Sep 6, 2019

codecov bot commented Sep 6, 2019

codecov bot commented Sep 6, 2019 •

edited

Loading

timholy left a comment

timholy Sep 6, 2019

Tokazama Sep 6, 2019

timholy Sep 6, 2019

timholy Sep 6, 2019

Tokazama Sep 6, 2019

timholy Sep 6, 2019

Tokazama Sep 6, 2019

timholy Sep 16, 2019

johnnychen94 Sep 16, 2019

timholy left a comment

timholy Sep 16, 2019

Tokazama Sep 16, 2019

timholy Sep 16, 2019

Tokazama Sep 16, 2019

timholy Sep 16, 2019

timholy commented Sep 16, 2019

Tokazama commented Sep 16, 2019

timholy commented Sep 16, 2019

Tokazama commented Sep 16, 2019

Tokazama commented Sep 16, 2019

Tokazama commented Dec 17, 2019

Tokazama commented Apr 23, 2020

	matching name is found any empy axis (i.e. `0:0`) is returned.
	matching name is found any empty axis (i.e. `0:-1`) is returned.

Added findaxis, finddim, and hasdim #89

Added findaxis, finddim, and hasdim #89

Conversation

Tokazama commented Sep 6, 2019

codecov bot commented Sep 6, 2019

Codecov Report

codecov bot commented Sep 6, 2019 • edited Loading

Codecov Report

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Sep 16, 2019

Tokazama commented Sep 16, 2019

timholy commented Sep 16, 2019

Tokazama commented Sep 16, 2019

Tokazama commented Sep 16, 2019

Tokazama commented Dec 17, 2019

Tokazama commented Apr 23, 2020

Added `findaxis`, `finddim`, and `hasdim` #89

Added `findaxis`, `finddim`, and `hasdim` #89

codecov bot commented Sep 6, 2019 •

edited

Loading